Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of input of type FormData for the Fetch client #1008

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

frazar
Copy link

@frazar frazar commented Dec 11, 2024

Several open issues are caused by the multipart/form-data formatter trying to provide a simple API based on an input of type Object. However, Object can not express some of the typical use cases of a multipart request. On the other hand, FormData could.

One example (mentioned in #705 and #965) is sending a requested with multiple entries belonging to the same "key". Since a FormData object allows one key to be mapped to multiple values, one can simply do:

f = new FormData()
f.append('files', new Blob())
f.append('files', new Blob())
[...f.keys()] // This gives 2 entries, as expected

This is clearly not possible with an Object, where the key-value relationship is 1:1. #1000 tries to achieve this by automatically converting Array values to multiple FormData keys. However, I argue that it is not necessarily what the user wants, since they could either expect:

  • that the array is sent as-is as a single FormData entry, or
  • that each array is sent a separate FormData entries.
    In principle, it is not so easy to infer which of the two is correct.

Currently, however, inputs of type FormData are not correctly handled by the multipart/form-data formatter for the Fetch client, which tries to enumerate the keys using the Object.keys(...) construct:

[ContentType.FormData]: (input: any) =>
Object.keys(input || {}).reduce((formData, key) => {

Unfortunately, FormData returns an empty list when used through Object.keys()

f = new FormData()
f.append('files', new Blob())
f.append('files', new Blob())
Object.keys(f) // This gives an empty array!

This PR proposes a way to allow the user to directly supply a FormData, basically opting out of any smart formatting logic that might actually get in the way. In this way, the user takes responsibility for correctly managing the conversion.

I argue that the breaking change is minimal since any use of FormData is broken under the current API. Additionally, this would align with the logic for the Axios client, which already short-circuits the formatting form FormData inputs.

protected createFormData(input: Record<string, unknown>): FormData {
if (input instanceof FormData) {

@frazar frazar changed the title Accept input of type FormData for the Fetch client Fix handling of input of type FormData for the Fetch client Dec 13, 2024
@smorimoto smorimoto requested a review from Copilot December 28, 2024 17:28

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 25 changed files in this pull request and generated no comments.

Files not reviewed (20)
  • templates/base/http-clients/fetch-http-client.ejs: Language not supported
  • tests/spec/another-query-params/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/custom-extensions/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/defaultAsSuccess/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/defaultResponse/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/deprecated/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/dot-path-params/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/enumNamesAsValues/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/extractRequestBody/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/extractRequestParams/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/extractResponseBody/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/extractResponseError/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/jsAxios/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/moduleNameFirstTag/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/moduleNameIndex/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/on-insert-path-param/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/patch/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/responses/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/singleHttpClient/snapshots/basic.test.ts.snap: Language not supported
  • tests/spec/sortTypes-false/snapshots/basic.test.ts.snap: Language not supported
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant